-
Notifications
You must be signed in to change notification settings - Fork 190
Alternative proposal to BEP038 #1856
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
cc @jdkent @melanieganz @CPernet @dorahermes @Remi-Gau @effigies @ericearl @francopestilli There are some wrinkles to iron out (e.g., missing glossary definitions breaking documentation building), but this is a general summary of how I see this. Happy to discuss use cases that are not immediately clear how they would be encoded under this proposal. Thank you all for your patience, this PR was long overdue. |
This comment was marked as resolved.
This comment was marked as resolved.
|
To ignore the arguments and boil this down to the practical difference between BEP38 and this counter-proposal, it seems to be:
As far as I can tell, anything that could be named under the existing BEP38 could be named under this (notwithstanding some comments on things that need clearing up below) proposal, so that's a good start. The last point I'm inferring just by its absence. Any recommendation on what people who saw value in this construct do? My personal inclination would be to use Some questions on your entities. I'll start with my understanding of how they seem to be used:
My understanding is that the 4-tuple I don't really understand I've only had a quick read-through and so I might have more thoughts later. I don't see any show-stopping problems, but I would like to hear from others who've been more in-the-weeds. Might be good to get people together in Seoul to discuss? |
|
One question regarding datatype under |
I think recommending
Yes, you're right -- tpl should be defined and it's not, I will address that ASAP. Controlled language - I see it as space in that it is semi-controlled. I would recommend using template space names from https://bids-specification.readthedocs.io/en/stable/appendices/coordinate-systems.html#standard-template-identifiers but allow any label if those standard names do not represent the data.
I don't know whether there's interest in maintaining another informal 'registry' like https://bids-specification.rtfd.io/en/stable/appendices/coordinate-systems.html#image-based-coordinate-systems? for spaces. My impression is that the spaces list has been pretty stable because the effect of adding new items is minimal. Perhaps this proposal should also have some sort of Otherwise, if there's a single file (e.g., a single Another interesting route would be to allow YAML to facilitate a natural language description of the methods of the atlas (i.e., embed a README into the metadata file). Some sort of Finally, it may be useful to have an I'm open to any suggestion to resolve this issue.
It is something else. It is common for atlases to define several levels (scales) of granularity of the defined ROIs. They are typically related hierarchically. E.g., say we have a parcellation that has 7 regions for each hemisphere at the lowest scale. Those regions are then divided in a number of regions at the next level, and so on up to dividing the hemisphere into 1000 ROIs in the highest scale. I think a very interesting paper that describes this as the choice of 'brain unit' is https://www.nature.com/articles/s41593-020-00726-z
I think this is a general question for BIDS Derivatives—by not saying anything explicit, we leave it open, and one day, BIDS Derivatives will address this issue. Validator-wise, I'd make it optional. |
|
Thanks for your work on this @oesteban! I largely agree with your approach. Scope of BEPAs a grounding for me (and hopefully for others), the Atlas BEP scope is to cover:
(as is the case with many/all derivatives) And an atlas can be created at either the:
But the atlas will always be applied to individual participant data.
|
|
Hi everyone, thanks for all your work on this @oesteban! As mentioned by @effigies, it would be great to also discuss this during the upcoming Brainhack if possible. @jdkent: how would @oesteban's proposal relate to the updates and examples you've worked on? It seems that both are more aligned than the previous BEP038 versions we had, no? Thanks again. Best, Peer |
|
Thanks for this proposal, @oesteban. I haven't had a chance to review it in detail yet, but will set aside some time next week to do so. For the PS-13 use case, at a high level, we are interested in 2 things:
Would this proposal be able accommodate that? |
Thanks for your feedback @jdkent. I think the above is the only caveat you found, so I'll go ahead and address your request with 'a little twist'. In the example, as it stands, the only metadata that can be generalized across items is However, generalization would be expected if two different template spaces are created (this is the twist). I've updated accordingly (see f159e61)
@PeerHerholz definitely :)
@pwighton —that's exactly the purpose. Yes, both are requirements of any BEP, and the proposal must abide by them. @effigies - I've tried to address some of your questions in 905160d. I'm afraid I'll need to keep working to make the specs render again. |
PeerHerholz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @oesteban! I think this looks great.
I was wondering if we should add a little bit of information concerning the different naming conventions, ie
tpl-MNI152NLin2009cAsym_from-MNI152NLin6Asym_mode-image_xfm.h5
vs.
sub-01_from-T1w_to-MNI152NLin2009cAsym_mode-image_xfm.h5
to prevent confusion in users (and other stakeholders). That's somewhat outside the scope of BEP038 but as BEP014 is still in development, a little explanation as to how certain transforms are named might be beneficial. WDYT?
I added a little mention to BEP014 in that commit: https://github.com/bids-standard/bids-specification/pull/1856/files#diff-930106228fdeff531c65486378dd4138c6f27c38cbce3bd7621743e4a42453e0R177-R179 I believe we should not attempt to get very deep into transforms here and let it happen within BEP14. |
Definitely! Sorry, I didn't mean to say that we should explain why there are different naming patterns for transform, just that they exist and refer to transforms between template spaces in one case and transforms between subject and template spaces in the other. Simply to avoid confusion. However, maybe that's just me, haha. |
src/derivatives/atlas.md
Outdated
| Please note that the specification for spatial transforms (BEP 014) is currently | ||
| under development, and therefore, the specification of transforms files may | ||
| change in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PeerHerholz this is the mention.
I didn't want to explicitly get down the from- / to- issue because that has great potential to change (or establish some extra rules so that it is unambiguous).
Thanks @oesteban! With that out of the way, I have a few minor comments:
Just curious what the role of the
|
|
That would be great! I'm available up to Tues Dec 17th. |
|
For those parties interested, please fill your availability at http://whenisgood.net/rrnttg4 |
|
@pwighton @mnoergaard @effigies let's meet on Friday (details for your timezone here) everyone is welcome to join :) |
|
Hey folks, I won't be able to make the meeting due to childcare. Best, Peer |
|
Hey everyone, I just wanted to ask how the meeting went and if it would be possible to get a brief summary, as well as if there are any tasks I could/should work on? Best, Peer |
|
@PeerHerholz we had a productive meeting. We discussed the main sticking points and found clearer ways of describing the two contended entities (tpl- and atlas-). Based on those discussions, @pwighton will create a new PR as soon as time and workload permit. We also thought that having more regular meetings (perhaps bi-weekly) starting in January will help push this forward. The feeling is that we are very close to wrapping up and final submission. EDIT: we had also a very productive exchange regarding when a new tpl- should be issued, in the case data are aligned into a pre-existing template space (e.g., fsaverage). We will also work making this clearer in the spec. Am I missing something? @effigies @mnoergaard |
|
Yes, I've just closed my PR and will pefrom another review and open a new PR after the holidays. I'm still a bit unclear on what the formal definition of an atlas and template will be. My understanding right now is:
@oesteban, let me know if those definitions seem like a good starting point for the review. I agree that regular meetings will be very helpful in finalizing this BEP and I'm looking forward to discussing this with you all in the new year. |
Thanks @oesteban and @pwighton does this solution look good to everyone? |
1 similar comment
Thanks @oesteban and @pwighton does this solution look good to everyone? |
If 'solution' means the arrangement described above, yes, it was a clear outcome of the meeting. If the solution is the final implementation of the BEP, then we are working on it, but we need to get a consensus on a few aspects where we still seem to think divergently despite having converged meaningfully for the most part. |
|
The ball has been in my court to re-work my PR in advance of the next meeting. I will try to get that done tomorrow or Friday at the very latest and then we can schedule the next meeting. Hopefully we can schedule a regular meeting (every 2 or 3 weeks perhaps) until the BEP is finalized Apologies to all for the delay. |
|
Hi all, A review of this proposal can be found here. For this review, as agreed upon at our last meeting, I've used the following working definitions:
My previous review of this proposal used the following working definitions:
We concluded that the working definitions used in the previous review were problematic, and I suspect we will come to the same conclusion here. Another definition, which we agreed upon in Copenhagen in 2023 was:
To which we could very naturally add the following definition for template:
I'm happy to consider other working definitions for these terms, and review the proposals accordingly, but it is clear that at some point we will have to define and execute some process by which we come to a consensus. Therefore, for our next meeting, I propose the following agenda:
Let me know what you think of this agenda or if other items should be added. If you would like to participate in the next meeting, please use this form: https://whenisgood.net/mcn39w7 Thanks to all and apologies again for the delay. |
|
Hey @pwighton, thanks so much for the update and all the information! I'll definitely try my best to join the meeting. Regarding achieving consensus: we tried voting periods in the past during which folks can provide 👍🏻 or 👎🏻 for different options and it seemed to work fine. Here's an example.. |
This reverts commit 891bcf0. The changes in that commit were made before our latter meetings by the end of 2024, and IMHO do not align with what we discussed. However, @mnoergaard's suggestions did address a specific issue we discussed in those meetings. For that reason, I'm keeping the commit in the history so that the specific suggestions are not lost, and I formally commit here to revisit the issue and add examples with Martin as co-author, re-including his suggestion.
|
ugh @effigies I merged master into this one without thinking much. Can you update the bep038 branch with master? I'm not allowed without a PR. |
Done. |
…n into bep038-review
Also, splits the schema definitions from "common_derivatives". cc/ @effigies
the new subsection should provide some guidance on how to encode the dataset to better reflect the methodological design behind the atlas generation. The new subsection should clarify the difference between generating the template in a standard space directly, such as MNI, or generating it in a study-specific space for your sample and later mapping those results into standardized spaces such as MNI.
|
To discuss tomorrow: I'm concerned that the |
Rendered proposal: https://bids-specification--1856.org.readthedocs.build/en/1856/derivatives/atlas.html
This is a proposal to achieve the aims of BEP 38 as derivative datasets, without the need to introduce a new
DatasetType. The structure of derivatives is well-established in BIDS, and this would require the introduction of fewer new concepts and less code complexity in supporting all BIDS DatasetTypes.To demonstrate, the proposal has several examples with 'classical' templates/atlases. In addition to that, I'm working on uploading PS13 to templateflow (further supporting the practical implementation of the proposal), and, if accepted, I can commit to generating bids-examples following the proposal for PS13.
I incorporate the BEP metadata from this spec, for the moment unchanged as the absence of macros made it really hard to carefully edit (that said, I think that part of the current proposal is okay, and I would possibly suggest some additions only).
This proposal only includes the following new entities:
As I understand it, the atlas BEP has taken the shape it has because it was felt that derivatives datasets would not satisfy the needs. I believe I have shown that it can with only minor modifications.
I include a skeleton of the new terms in this PR. If we agree in principle, we can work on schematizing these changes and tightening up the text.
My proposal is issued to address three central issues and other relevant problems. For the interested, I include arguments against specific choices in the BEP as-is, but I hope my arguments for this proposal stand on their own. To see these arguments, please unfold this paragraph by clicking the initial arrow.
Issue 1: Opening
DatasetTypeto values other thanrawandderivativesshould have its own BEP.Adding values to
DatasetTypeis a major change to the specification that should be broadly discussed by the community, with a preliminary analysis of potential side-effects by the SG and/or Maintainers.It took a long while before
derivativesbecame a relevant part of BIDS and many years of discussion about them. I contend thatDatasetTypeshould keep its special status and be discussed separately. AfterDatasetTypeis agreed as the appropriate mechanism, the BEP leads intending to add values should state it when presenting the BEP draft to the SG before it becomes listed as an active BEP.For instance, it would not be crazy to contemplate the possibility of having a
DatasetTypesuch asfreesurfer, which has a very stable and standardized data structure, to allow it as a standalone dataset. OpeningDatasetTypemeans opening BIDS to the creation of standards within the standard. Where to draw the line betweenrawandderivativehas traditionally be a contention point, so enabling more options should be considered very carefully, and provided with prescriptions of how to do it and how to decide beforehand. Otherwise, BEPs proposing new dataset types will creep up as we all tend to think that our area of specialization is special.Please note that this issue does not enter into the actual value of
atlasproposed by the BEP. That is reviewed next.Proposed solution: (1) drop this part of the proposal; (2) discuss the issue as BIDS prescribes; (3) establish whether the intent of
DatasetTypemay be open to other dataset types.Issue 2: The new value
atlasforDatasetTypeevades the actual problem.Evading *the* problem that exists. By creating the new
DatasetTypemetadata, the overarching problem is escaped: the fact that BIDS-Derivatives has not been developed far enough to represent "second-level" analyses, as in, analyses where data from several subjects, or sessions, or runs, are pooled together. Instead, the current BEP proposal cordons off the problem by creating its own little island.Solving a problem that does not exist. The use of the new
DatasetTypeis justified to enable the sharing of "atlas", as stated in the initial paragraph, and later:which suggests that, if a dataset is of
derivativetype then the following is not supported:derivativedataset cannot be integrated as a sub-dataset of another BIDS dataset (which is factually false).Therefore, this approach seems to indicate that atlases are somewhere in between "raw" and "derivative" and hence they require their own
DatasetType.Proposed solution: My proposal encodes atlas-derived results and atlas-generating pipelines results within current BIDS-derivatives specifications. If I'm reviewing a paper corresponding to a new template and/or atlas, I would feel better equipped to understand the pipeline and the results if delivered as BIDS-Derivatives, with the most salient intermediate steps there (or transformations so that I can replicate them) instead of a final structure that looks like templateflow's resources putting
atlas-first. The first reports the atlas creation process, while the second is a fast-track mechanism to emancipate the blobs a researcher wants be reused from the outputs and reporting of the generating pipeline. My understanding of BIDS is that it wants to achieve the first. The act of sharing data and ensuring FAIRness in the delivery of the service is more of a responsibility of other players such as OpenNeuro or TemplateFlow.Issue 3: the folder structure is inconsistent with current BIDS raw and derivatives
This PR proposes an alternative that is consistent with current BIDS. While for raw and first-level analyses derivatives the spatial reference is established by that of individual subjects, for higher-than-first-level analyses this PR proposes the concept of template, which is the aggregation of feature maps that serve for reference at the individual level (e.g., aggregation of runs, sessions or sets of subjects). That allows for a more consistent organization, which has been already tested in the wild with TemplateFlow.
In addition, there are several aspects of atlases (and templates) that this BEP did not cover:
Problem 1: longitudinal templates (and atlases)
The cohort entity of templateflow could resolve this. I can update my PR if it is accepted to contemplate this.
Problem 2: multi-scale atlases
My proposal includes a new scale- entity.
Problem 3: probabilistic surface parcellations.
This would require finding a GIFTI encoding of FreeSurfer's GCS format. This is not really a problem of atlas, but BIDS-Derivatives in general.
Proposed solution: Implemented by this PR against BEP038.
Other issues
Downstream problems of the proposed
DatasetType. It seems the intent is to have these datasets uploaded to BIDS-compatible platforms such as OpenNeuro as a new means of disseminating and distributing atlases. OpenNeuro does implement FAIR pretty comprehensively, which is fundamental for this intent not to become extremely dangerous, but at the outskirt, the BIDS specifications should refrain from suggesting OpenNeuro should be used for sharing. These atlases will likely be shared through other venues where data versioning, accessibility, etc. are not as transparent or available and that will have the opposite effect that is intended in this BEP (undermined reproducibility and limited reusability of the atlas). But even assuming OpenNeuro as the mechanism for redistribution, there are other issues that are covered in our TemplateFlow paper, which will be problematic if not exacerbated:atlasentity, and I don't think it would be good for BIDS to attempt to control that. The experience would revive the issues hit with template specifications (https://bids-specification.readthedocs.io/en/stable/appendices/coordinate-systems.html). I also provided an example of this problem within BEP Proposal: Atlas specification #1281.DatasetTypeatlasallows people to mark a resource as atlas and confusingly set no-derivs (and maybe request royalties after use?). Forderivativeit is not assumed that you can create further derivatives and the license is checked.Intro of the proposal misses the point. The introduction of the current proposal is largely devoted to explain what an atlas is. BIDS should not be a neuroimaging handbook, and therefore, BEPs should not require such justifications. I believe this is a consequence of issue 2 to justify the choice.